Conversation
- throttle의 마지막 이벤트 방출을 추가로 해주는 latest의 기본값이 true임 - 다음 페이지를 호출하는 이벤트를 시작과 끝에 두번 호출이 되게 하면 페이지 요청이 중복적으로 들어가는 문제와 설계에 따라서는 다음 페이지까지도 불러올 수 있기에 latest 옵션을 false로 변경
- 불필요한 상황에서 모두 loadView를 mutate하고 있었음 - 아마 if state가 아닌 상황들에서 return이 없다보니 complete가 아닌 영향을 크게 주지 않는 loadView를 호출할게 아닌가 싶음.
- 50개나 요청할 필요가 없다 생각했음 - 한번에 보이는 팝업의 수량이 그리 많지 않았음 - 이미 충분히 카테고리와 정렬로 최상단의 정보의 중요도가 더 높다 판단했음 - 고로 그 아래 50개씩이나 불러올 이유는 없다고 보였음
- 단순히 .init으로 설정해주면 return이 뭘로 되어있는지 확인을 해줘야되는 번거로움이 있음
- 한번 생성자로 만들어준 아이템은 저장하도록 함 - 이후 조건에 따라 아이템으로 밀어줄건지 추가해줄건지만 정함
- sections에서 HomeGridSection을 찾음 - HomeGridSection에 새로 추가될 아이템을 넣어줌 - controller가 해당 아이템을 넣어줄 섹션을 알려주기 위한 indexPath 전달
zzangzzangguy
approved these changes
May 1, 2025
Contributor
zzangzzangguy
left a comment
zzangzzangguy
left a comment
There was a problem hiding this comment.
전체적으로 코드 가독성이 좋아지고
페이지네이션 로직이 더 견고해진걸로 보입니다 수고하셨습니다 💪
| private let cellTapped: PublishSubject<IndexPath> = .init() | ||
| private let pageChange: PublishSubject<Void> = .init() | ||
| private let loadNextPage = PublishSubject<Void>() | ||
| } |
Contributor
There was a problem hiding this comment.
변수명을 더 직관적으로 변경하셨군요 😂
| .forEach { self.view.addSubview($0) } | ||
| } | ||
|
|
||
| func setupContstraints() { |
Member
Author
There was a problem hiding this comment.
Contributor
There was a problem hiding this comment.
configureIUI와 addViews에서 뷰추가가 중복되있는것 같습니다!
Member
Author
There was a problem hiding this comment.
앗! 저는 재사용 셀 등록은 뷰를 추가하는 과정은 아니라고 생각해서 configureUI에 등록을 해뒀던 거였습니다 👍🏻
이 부분은 생각의 차이가 있을 수 있어서 다음 회의때 어떤 스타일로 사용할건지 의논하고 하나의 스타일로 병합하면 좋을것같아요!
dongglehada
approved these changes
May 1, 2025
Member
dongglehada
left a comment
dongglehada
left a comment
There was a problem hiding this comment.
UI 관련 코드의 클로저도 then으로 변경이 되었군요..! 까먹고 있었는데 꼼꼼하십니다.! 내용은 확인 하였습니다 :>
| let contentOffsetY = scrollView.contentOffset.y | ||
| if contentOffsetY + scrollViewHeight >= contentHeight { | ||
| pageChange.onNext(()) | ||
| loadNextPage.onNext(()) |
Member
Author
There was a problem hiding this comment.
아마 몇개셀 이전에 돌아가도록 하면 좋긴 하겠는데, 추후 새로운 피쳐로 재구성을 염두하고 있어서 그런지 불필요한 작업이라 판단해서 추후 작업할때 반영해볼것 같아요!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tip
이번 PR은 commit 단위로 넘겨가며 보셔도 좋다 생각합니다.
작업 사항이 좀 복잡했었다보니 commit마다 description과 제목을 확인해주세요 👍🏻
📌 이슈
✅ 작업 사항
🚀 테스트 방식
기존의 스크롤뷰를 데이터가 많은 페이지(종료된 필터 이용)로 이동한 뒤 스크롤을 마구 해봐주세요 👍🏻
👀 ETC (추후 개발해야 할 것, 참고자료 등) ->
스크롤뷰의 튕김 문제를 해결하는 과정에서 구조적인 한계를 많이 느꼈던것 같습니다.
오류와 관련된 트러블슈팅 로그와 느꼈던 점들은 해당 링크에서 확인하실 수 있습니다.
또한 테스트를 하기 위해서 필요한 뎁스가 어느정도 있던 작업이었다보니 테스트에 있어서 불리한 구조를 개선하고자 별도의 Feature 모듈과 Demo를 도입하기에도 좋다 생각했습니다.
현재의 기능을 확인하기 위해서는
홈 → 검색창 → 필터 변경 → 스크롤의 순서로 반복적으로 잡업을 해야됐고, 이는 테스팅을 위한 환경 제공, 데모앱으로 추후 검색 기능이 변경됨에도 유리해질 수 있다 생각합니다.